Skip to content

chore(OC_App): migrate more legacy function and usage#60071

Merged
susnux merged 2 commits into
masterfrom
chore/legacy
May 14, 2026
Merged

chore(OC_App): migrate more legacy function and usage#60071
susnux merged 2 commits into
masterfrom
chore/legacy

Conversation

@susnux
Copy link
Copy Markdown
Contributor

@susnux susnux commented May 3, 2026

Summary

Move more functions to AppManager that still make sense to be shared.
Moreover usage is migrated to AppManager instead of legacy OC_App.

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@susnux susnux added this to the Nextcloud 34 milestone May 3, 2026
@susnux susnux added 3. to review Waiting for reviews technical debt 🧱 🤔🚀 ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels May 3, 2026
@susnux susnux force-pushed the chore/legacy branch 3 times, most recently from 031c44c to 8c22c09 Compare May 5, 2026 21:27
@susnux susnux marked this pull request as ready for review May 6, 2026 08:15
@susnux susnux requested a review from a team as a code owner May 6, 2026 08:15
@susnux susnux requested review from leftybournes, nfebe, salmart-dev and sorbaugh and removed request for a team May 6, 2026 08:15
Comment thread lib/private/App/AppManager.php
@susnux susnux requested a review from artonge May 6, 2026 10:16
@susnux susnux requested a review from provokateurin May 6, 2026 16:39
Copy link
Copy Markdown
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few small things that could be improved.

Comment thread lib/private/App/AppManager.php Outdated
Comment thread lib/private/App/AppManager.php Outdated
Comment thread lib/private/App/AppManager.php Outdated
Comment thread lib/private/App/AppManager.php Outdated
* This function walks through the Nextcloud directory and loads all apps
* it can find. A directory contains an app if the file `/appinfo/info.xml`
* exists.
*
* if $types is set to non-empty array, only apps of those types will be loaded
* @param string[] $types - If set, only apps of these types will be loaded
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param string[] $types - If set, only apps of these types will be loaded
* @param list<string> $types - If set, only apps of these types will be loaded

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a "breaking change" because then the function requires more specific parameter.
I think functions should take arrays as param but return list, so they allow more flexible usage while return type is specific to ease using it.

Especially here its valid to pass an array, we handle it properly

Comment thread lib/private/App/AppManager.php Outdated
Comment thread lib/private/App/AppManager.php Outdated
Comment thread lib/private/App/AppManager.php
Comment thread lib/private/legacy/OC_App.php Outdated
Comment thread lib/public/App/IAppManager.php Outdated
Comment thread lib/private/App/AppManager.php Outdated
…o AppManager

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux dismissed provokateurin’s stale review May 13, 2026 20:18

All changes implemented

@susnux susnux merged commit 9579170 into master May 14, 2026
277 of 298 checks passed
@susnux susnux deleted the chore/legacy branch May 14, 2026 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) technical debt 🧱 🤔🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants